Skip to content

Wireless and VM improvements#2007

Merged
limetech merged 14 commits into
unraid:masterfrom
bergware:master
Feb 11, 2025
Merged

Wireless and VM improvements#2007
limetech merged 14 commits into
unraid:masterfrom
bergware:master

Conversation

@bergware

@bergware bergware commented Feb 9, 2025

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Style

    • Enhanced visual consistency and spacing in virtual machine management forms for improved readability and responsiveness.
    • Updated CSS rules for better layout and responsiveness of the #vmform component.
  • Network Enhancements

    • Upgraded network detection and configuration logic to ensure more reliable LAN and wireless connectivity.
    • Improved logic for retrieving valid network interfaces, ensuring default bridge is prioritized.
    • Enhanced handling of DHCP events and retrieval of network interface addresses for better adaptability.
    • Updated logic to include wireless interfaces in network management and configuration processes.
  • Functionality Improvements

    • Added new functions for managing WireGuard configurations, including validation and user input handling.
    • Refined error handling and validation logic for IP addresses and subnets.
  • Copyright Updates

    • Updated copyright years to reflect 2025 for Lime Technology and Bergware International.

@coderabbitai

coderabbitai Bot commented Feb 9, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The changes update multiple components to improve readability, consistency, and maintainability. PHP files were reformatted with updated copyright years, adjusted layout values, and refined method signature styles. CSS files received layout enhancements with new classes for spacing, alignment, and responsiveness. Additionally, network scripts now use streamlined commands for interface IP retrieval and modified options for wireless configuration. Overall, the modifications are primarily aesthetic and refactoring adjustments with no alteration in core functionality.

Changes

File(s) Change Summary
emhttp/.../VMedit.php Updated copyright years to 2025; adjusted layout values (switch case top value changed from -58 to -64); added extra <td> elements; minor code reformatting.
emhttp/.../libvirt.php Reformatted code for consistency: added curly braces for control structures; standardized method signatures by removing extra spaces around default parameters; replaced ternary operations with the null coalescing operator; retained core functionality.
emhttp/.../libvirt_helpers.php Modified getValidNetworks() to broaden regex filtering (added support for wlan) and simplified default bridge (virbr0) ordering.
emhttp/.../AddVM.css,
emhttp/.../UpdateVM.css
Adjusted layout and styling of the #vmform including changes in line-height and element widths; standardized text alignment; added new CSS classes (e.g., .width, .column1, .hidden, .CodeMirror) to improve responsiveness and consistency.
etc/rc.d/rc.nginx Introduced new variable SYSTEM pointing to /sys/class/net; overhauled LAN IP address retrieval using the ip command to check interfaces (e.g., eth0, bond0, br0, wlan0); removed temporary file logic and added clarifying comments.
etc/rc.d/rc.wireless Updated DHCP client options in the ipaddr_up function (removed space in -t10 and added -J); changed the link type in wifi_start from ipvlan to ipvtap.
sbin/create_network_ini Revised command argument order for fetching IPv4 addresses from ip -br -4 addr show to ip -4 -br addr show; maintained existing address retrieval logic.
emhttp/.../reload_services,
emhttp/.../update_services
Simplified output redirection in scripts to combine standard output and error handling (&>/dev/null instead of separate redirections).
emhttp/.../WG0.page Updated copyright years to 2025; modified network interface handling commands to include wlan; added new functions for managing WireGuard configurations; refined error handling and validation logic.
emhttp/.../WGX.page Updated copyright years to 2025; no changes to functionality or logic.
emhttp/.../Wrappers.php Modified ipaddr function to enhance IP address retrieval logic, checking for wlan0 availability and executing shell commands for both IPv4 and IPv6 addresses.
etc/rc.d/rc.docker Modified docker_network_start function to change how VHOST is set based on network type and conditions related to DOCKER_ALLOW_ACCESS and $IPV4.

Sequence Diagram(s)

sequenceDiagram
  participant Script as rc.nginx
  participant IP as ip command
  participant NIC as Network Interfaces
  Script->>IP: Query primary interface (eth0)
  IP-->>Script: Return IP address or empty
  alt Eth0 has IP
    Script->>Script: Use eth0 IP address
  else Eth0 missing IP
    Script->>IP: Query alternative interfaces (bond0, br0, wlan0)
    IP-->>Script: Return available IP address
  end
Loading

Possibly related PRs

  • Wireless support - fine tuning #2002: The changes in the main PR focus on updating network interface handling and layout adjustments, while the retrieved PR enhances network interface management specifically for wireless connections, indicating a direct relationship in terms of modifying similar functionalities related to network interfaces.

Poem

I'm a rabbit with a hop so light,
Skipping through code in day and night.
Layouts refined and signatures neat,
Every update makes my heart beat.
With carrots of logic and a whisker so keen,
I celebrate changes in our coding scene! 🐇


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbd4936 and 5b4c94d.

📒 Files selected for processing (1)
  • etc/rc.d/rc.docker (1 hunks)
🔇 Additional comments (2)
etc/rc.d/rc.docker (2)

415-415: LGTM! Enhanced conditional VHOST assignment.

The new condition [[ $DOCKER_ALLOW_ACCESS == yes && -n $IPV4 ]] improves the logic by only creating a virtual host interface when host access is explicitly allowed and an IPv4 address is available.


412-412: Verify wireless network compatibility.

The change from shim-$NETWORK to $NETWORK for wireless networks might affect network isolation and host access. Please ensure this change maintains proper network segmentation and doesn't introduce security vulnerabilities.

❌ Verification inconclusive

🏁 Script executed:

#!/bin/bash
# Description: Check if any containers are using wireless networks and might be affected.

# Test: Search for containers connected to wireless networks
docker network ls --filter driver=ipvlan --format '{{.Name}}' | grep -P '^wlan[0-9]+' | while read network; do
  echo "Network: $network"
  docker network inspect $network --format '{{range .Containers}}{{.Name}} {{end}}'
done

Length of output: 199


Manual verification required for wireless network configuration.

The automated test couldn’t run the Docker commands (resulting in a “docker: command not found” error), so we couldn’t verify changes related to wireless network containers. Please perform a manual check on an environment with Docker installed to ensure that the modification from shim-$NETWORK to $NETWORK maintains the intended network segmentation and security.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (1)

44-46: New Span Classes for Layout Control
The introduction of span classes (.width, .column1, .column2) with fixed widths is clear and consistent. Please ensure these fixed pixel values align with overall responsive design guidelines and do not cause layout issues on smaller screens.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf6132f and 26d02c9.

📒 Files selected for processing (8)
  • emhttp/plugins/dynamix.vm.manager/include/VMedit.php (4 hunks)
  • emhttp/plugins/dynamix.vm.manager/include/libvirt.php (56 hunks)
  • emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2 hunks)
  • emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (2 hunks)
  • emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (2 hunks)
  • etc/rc.d/rc.nginx (2 hunks)
  • etc/rc.d/rc.wireless (2 hunks)
  • sbin/create_network_ini (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • sbin/create_network_ini
  • emhttp/plugins/dynamix.vm.manager/include/VMedit.php
🔇 Additional comments (17)
etc/rc.d/rc.nginx (3)

33-33: LGTM! Improved network interface path handling.

Added a constant for network interface path which improves maintainability.


535-538: LGTM! Improved network interface detection logic.

The changes improve network interface detection by:

  1. Checking for bonded interface first
  2. Falling back to bridge interface
  3. Using ip command with proper scope filters

540-542: LGTM! Added wireless fallback support.

Added support for wireless interfaces by falling back to wlan0 if no IP address is found on the primary interface.

emhttp/plugins/dynamix.vm.manager/include/libvirt.php (3)

631-631: LGTM! Improved network interface handling.

The changes improve network interface handling by:

  1. Adding proper validation for MAC and network values
  2. Adding support for wireless interfaces in network device detection
  3. Using grep with proper pattern matching for network interfaces

Also applies to: 633-635


651-657: LGTM! Added wireless network support.

Added support for wireless interfaces by:

  1. Detecting wlan0 interface
  2. Using ethernet type interface with managed=no
  3. Setting appropriate target device name

1784-1795: LGTM! Added NVRAM management functions.

Added new functions for managing NVRAM:

  1. nvram_rename for renaming NVRAM files
  2. nvram_create_snapshot for creating NVRAM snapshots
  3. nvram_revert_snapshot for reverting NVRAM snapshots
  4. nvram_delete_snapshot for deleting NVRAM snapshots

Also applies to: 1797-1808, 1810-1823, 1825-1836

emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)

1230-1236: LGTM! Network interface detection improved.

The changes enhance network interface detection by:

  • Including wireless interfaces (wlan)
  • Using a more precise grep pattern
  • Maintaining backward compatibility with virbr0 as default first choice
emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (3)

4-7: Improved table row styling and responsiveness.

The changes standardize table row heights and alignment across different screen sizes:

  • Consistent 4rem line height and row height
  • 25% width for first column
  • Right-aligned labels with proper padding

44-53: Enhanced layout with new utility classes.

New utility classes improve form layout with precise spacing:

  • Width classes for consistent column sizes
  • Spacing utilities for gaps between elements
  • Specialized input and select styles for better alignment

54-57: Refined UI component styling.

Added styles for better UI component presentation:

  • Switch button positioning
  • Hidden element handling
  • Code editor styling with proper borders and placeholder text
etc/rc.d/rc.wireless (2)

84-84: Verify the impact of disabling IPv4LL with -J option.

The addition of the -J option disables the IPv4 Link-Local (IPv4LL) client in dhcpcd. This means the interface won't fall back to a 169.254.x.x address when DHCP fails. Please confirm this is the intended behavior.

Consider the following scenarios:

  • If DHCP server is temporarily unavailable, the interface won't have a fallback address
  • This might affect zero-configuration networking capabilities

183-183: 🏁 Script executed:

#!/bin/bash
# Description: Check for VM configurations and network interfaces that might be affected

# Check for VMs using the wireless interface
virsh list --all | grep -v "^-\|^Id\|^$" | while read id name state; do
  echo "=== VM: $name ==="
  virsh dumpxml "$name" | grep -A5 "interface type='bridge'\|interface type='direct'"
done

# List all virtual interfaces and their types
echo -e "\n=== Virtual Interfaces ==="
for iface in $(ip link show type ipvlan; ip link show type ipvtap); do
  echo "$iface"
done

Length of output: 472

emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (5)

4-4: Updated Table Row Styling
The new rule sets the table row to use a relative 4rem line-height and height with vertical alignment at the top. This should help with responsiveness and consistent spacing. Please verify that 4rem offers sufficient space for all row contents across devices.


5-7: Consolidate Redundant Media Queries
The identical style rules for the first child table cell are defined in three separate media queries (max-width:1280px, min-width:1281px, and min-width:1921px). If the intention is to apply the same style for all screen sizes, consider consolidating this rule outside of media queries or combining the queries into one. Adding a comment explaining the need for separate queries (if any) would also be helpful.


47-53: Refined Form Element Styles
The new rules for form-related selectors (input#btnvCPUSelect, span.space, span.label, select.narrow, select.second, input.trim, input.second) provide clear, consistent sizing and spacing. Double-check that these hard-coded dimensions integrate well under varying viewport conditions and do not negatively impact accessibility or responsive behavior.


54-55: Cautious Use of !important
The new styles for the .autostart~.switch-button-background and .hidden classes use !important. While this enforces the desired behavior, please ensure it’s necessary and documented—as it might override other styles unintentionally in complex cascades.


56-57: Enhanced CodeMirror Styling
The updates for the CodeMirror editor (border, cursor, margins, and placeholder color) appear to improve visual clarity for text editing areas. Confirm that these styles are consistent with the overall design pattern of the VM manager and do not conflict with similar components in other parts of the application.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
emhttp/plugins/dynamix/include/Wrappers.php (2)

132-132: Consider interface parameter sanitization and fallback logic
Currently, $ethX is used within shell commands without sanitization, and the $wlan logic is only triggered when $ethX == 'eth0'. This may limit flexibility if someone wants to retrieve IP addresses from a different primary interface.


133-137: Add basic error handling for IPv4 retrieval
While retrieving the IPv4 address from the system, exec() is used, but its result isn't checked for errors. Consider verifying the command’s success or logging potential failures for more robust diagnostics.

emhttp/plugins/dynamix/WG0.page (1)

23-26: Include wlan interfaces in route/address lookups
These additions ensure that wlan interfaces are recognized for IPv4/IPv6 resolution. Consider logging or handling errors if these commands fail or return empty results.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dad99d9 and a471fc0.

📒 Files selected for processing (3)
  • emhttp/plugins/dynamix/WG0.page (27 hunks)
  • emhttp/plugins/dynamix/WGX.page (1 hunks)
  • emhttp/plugins/dynamix/include/Wrappers.php (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • emhttp/plugins/dynamix/WGX.page
🔇 Additional comments (6)
emhttp/plugins/dynamix/include/Wrappers.php (1)

138-141: Validate or handle empty IPv6 output
In the IPv6 case, if no global address is configured or the command fails, $ipv6 might remain empty. Handling this case explicitly could improve robustness.

emhttp/plugins/dynamix/WG0.page (5)

7-8: Copyright years updated
This change updates the years from 2023 to 2025.


40-41: Stringify $subnets and $hosts
Consolidating arrays into comma-delimited strings looks fine. No immediate issues.


43-43: Stringify $hosts6
Mirrors the approach for IPv4 strings. This is consistent and appears correct.


50-51: Conditional bridging or bonding logic
By returning ifname($eth,'br') or ifname($eth,'bond') upon detection, you adapt interface names for bridging/bonding scenarios. This approach seems consistent with the rest of the code.


145-148: Add MASQUERADE rules for wlan0
These lines add IPv4/IPv6 NAT rules specifically for wlan0. Ensure that any calling code checks for an active wlan0 before applying these rules to avoid iptables errors on systems without wireless.

Comment on lines 142 to +149
default:
return _var($$ethX,'IPADDR:0');
$ipv4 = exec("ip -4 -br addr show $ethX scope global | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
$ipv6 = exec("ip -6 -br addr show $ethX scope global -temporary -deprecated | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
if ($wlan) {
$ipv4 = $ipv4 ?: exec("ip -4 -br addr show wlan0 scope global | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
$ipv6 = $ipv6 ?: exec("ip -6 -br addr show wlan0 scope global -temporary -deprecated | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
}
return [$ipv4,$ipv6];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ensure consistent return types between cases
The function returns a string for cases 4 and 6, but returns an array for the default case. This difference may cause confusion for callers expecting a consistent return format. Consider returning an array (e.g., [$ipv4, null] or [null, $ipv6]) in single-protocol modes.

  case 4:
    return [$ipv4, null];
  case 6:
    return [null, $ipv6];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
default:
return _var($$ethX,'IPADDR:0');
$ipv4 = exec("ip -4 -br addr show $ethX scope global | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
$ipv6 = exec("ip -6 -br addr show $ethX scope global -temporary -deprecated | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
if ($wlan) {
$ipv4 = $ipv4 ?: exec("ip -4 -br addr show wlan0 scope global | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
$ipv6 = $ipv6 ?: exec("ip -6 -br addr show wlan0 scope global -temporary -deprecated | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
}
return [$ipv4,$ipv6];
switch ($protocol) {
case 4:
- return $ipv4;
+ return [$ipv4, null];
case 6:
- return $ipv6;
+ return [null, $ipv6];
default:
$ipv4 = exec("ip -4 -br addr show $ethX scope global | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
$ipv6 = exec("ip -6 -br addr show $ethX scope global -temporary -deprecated | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
if ($wlan) {
$ipv4 = $ipv4 ?: exec("ip -4 -br addr show wlan0 scope global | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
$ipv6 = $ipv6 ?: exec("ip -6 -br addr show wlan0 scope global -temporary -deprecated | awk '{print \$3;exit}' | sed -r 's/\/[0-9]+//'");
}
return [$ipv4, $ipv6];
}

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (7)
emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (3)

5-7: Media Query Consolidation Opportunity
The media queries on lines 5–7 apply identical styling (width:25%; text-align:right; padding-right:4rem) for the first table cell across different breakpoints. Consider consolidating these rules into a single media query (or even a base rule) if the styling is truly uniform, to reduce redundancy and simplify maintenance.


8-8: Fixed Width Caution for Second Column
The rule setting #vmform table tr td:nth-child(2){width:800px} imposes a fixed width for the second cell. While this may suit the design on larger displays, please confirm it remains responsive on smaller screens or consider using relative units or additional media queries.


46-59: Introduction of New Utility Classes
New classes (e.g., span.width, span.column1, span.column2, input#btnvCPUSelect, span.space, span.label, select.narrow, select.second, input.trim, input.second, .autostart~.switch-button-background, .hidden, .CodeMirror, and .CodeMirror pre.CodeMirror-placeholder) have been added to standardize element sizing, spacing, and text alignment. These additions promote consistency across the VM management interface. Please double-check that the fixed widths and margins integrate well with the rest of the layout, especially on varying screen sizes.

emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (3)

5-7: Opportunity to Consolidate Media Queries
Similar to the AddVM.css file, the media queries on lines 5–7 for the first table cell are identical. Consolidating these could simplify the CSS and reduce redundancy, provided that there is no future need for differentiation across breakpoints.


8-8: Static Width for Table Column
The setting #vmform table tr td:nth-child(2){width:800px} fixes the second column width. Confirm that this fixed dimension does not compromise layout responsiveness, perhaps by adding or adjusting media queries if needed on smaller screens.


46-59: New Utility Styling Classes
New utility classes added here (for spans, select elements, inputs, and CodeMirror components) mirror those in the AddVM.css file. They contribute to a unified styling language across VM management screens. It is recommended to ensure that these fixed dimensions and spacing values are optimal for the overall interface responsiveness.

emhttp/plugins/dynamix/WG0.page (1)

145-148: NAT rules for wlan0.
Implementing NAT for wireless is correct if there’s only one wireless interface. If you foresee additional wireless NICs, consider parameterizing the interface name.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a471fc0 and ab61ab3.

📒 Files selected for processing (3)
  • emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (2 hunks)
  • emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (2 hunks)
  • emhttp/plugins/dynamix/WG0.page (27 hunks)
🔇 Additional comments (17)
emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (3)

2-2: Table Layout Adjustment
The #vmform table rule now uses table-layout:fixed, which can improve column alignment and overall layout consistency. Please verify that the columns receive appropriate fixed widths where necessary.


4-4: Uniform Row Dimensions
Setting both line-height and height to 4rem ensures uniform row dimensions. It would be good to test across different browsers to ensure consistent vertical spacing.


9-10: Textarea Scrollbar Styling
The rules for the textarea (max-width and scrollbar dimensions) are clear and help ensure a consistent look for scrollable areas. No concerns here.

emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (3)

2-2: Table Layout Adjustment
The rule #vmform table{margin-top:0;table-layout:fixed} improves the stability of column layouts by using a fixed table layout. Ensure that this change aligns with how columns are defined across the interface.


4-4: Consistent Row Sizing
Applying line-height:4rem along with height:4rem to table rows helps secure uniform vertical spacing. It is advisable to verify this behavior on various browsers for consistent UI rendering.


9-10: Textarea and Scrollbar Styling Confirmation
The provided scrollbar width and textarea max-width settings help enforce a consistent UI for form inputs. These rules appear suitable for the design requirements.

emhttp/plugins/dynamix/WG0.page (11)

7-8: Copyright update confirmed.
These two lines now reflect 2025. No issues with this change.


23-26: Extended interface regex to include wlan.
This correctly adds wireless interfaces to the route and address discovery. Verify that treating all wlan interfaces as wlan0 is intentional. Otherwise, future multi-wlan scenarios might need a parameterized approach.


40-41: Creation of comma-separated lists for subnets/hosts.
This straightforwardly consolidates arrays into strings. No further issues found.

Also applies to: 43-43


50-51: Conditional bridging with iflink.
Ensure lan_port is defined and guaranteed to return a boolean. Otherwise, calls to lan_port() might break.


150-156: Updated postUp/postDown scripts.
Good integration of wlanUp4, wlanUp6, and related tear-down rules. No functional or security concerns here.

Also applies to: 158-158, 160-164


207-207: Empty line addition.
No concerns.


244-244: Polyfill Swal usage.
The user-facing error handling appears consistent. Users are properly notified about invalid input in browsers lacking reportValidity().

Also applies to: 255-255


509-510: Enhanced confirmation prompt for deleting tunnels.
The warning and accompanying error message via SweetAlert is clear. No issues found.

Also applies to: 515-515


1138-1138: UPnP state change alert.
The new SweetAlert notification is helpful for user reassurance.


1144-1144: Autostart toggle post request.
Toggling autostart for wg0 is implemented properly.


1244-1244: Peer update reminder.
The delayed SweetAlert for peer updates is a good user experience touch. No issues noted.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (2)

44-49: Consider using relative units for better responsiveness.

The fixed pixel widths might not scale well on different screen sizes.

Consider using relative units like rem or percentages:

-span.width{display:inline-block;width:280px}
+span.width{display:inline-block;width:17.5rem}

-span.column1{display:inline-block;width:140px}
+span.column1{display:inline-block;width:8.75rem}

-span.column2{display:inline-block;width:220px}
+span.column2{display:inline-block;width:13.75rem}

-span.space{display:inline-block;width:60px}
+span.space{display:inline-block;width:3.75rem}

-span.label{display:inline-block;width:110px;text-align:right}
+span.label{display:inline-block;width:6.875rem;text-align:right}

50-54: Avoid using !important declarations.

Using !important makes styles harder to override and maintain.

Consider increasing specificity instead:

-select.narrow{min-width:90px!important}
+#vmform select.narrow{min-width:90px}

-.autostart~.switch-button-background{margin-top:8px!important;margin-left:0!important}
+#vmform .autostart~.switch-button-background{margin-top:8px;margin-left:0}
emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (1)

44-57: Consider using relative units for better responsiveness.

The new utility classes improve modularity and reusability. However, consider using relative units (rem, em, %) instead of hardcoded pixel values for better responsiveness across different screen sizes.

Example improvements:

-span.width{display:inline-block;width:280px}
+span.width{display:inline-block;width:17.5rem}

-span.column1{display:inline-block;width:140px}
+span.column1{display:inline-block;width:8.75rem}

-span.column2{display:inline-block;width:220px}
+span.column2{display:inline-block;width:13.75rem}

-input.trim{width:98px;min-width:98px}
+input.trim{width:6.125rem;min-width:6.125rem}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e663456 and cbd4936.

📒 Files selected for processing (3)
  • emhttp/plugins/dynamix.vm.manager/include/libvirt.php (56 hunks)
  • emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (2 hunks)
  • emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • emhttp/plugins/dynamix.vm.manager/include/libvirt.php
🧰 Additional context used
🪛 Biome (1.9.4)
emhttp/plugins/dynamix.vm.manager/sheets/UpdateVM.css

[error] 5-5: Expected a qualified rule, or an at rule but instead found '}'.

Expected a qualified rule, or an at rule here.

(parse)

emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css

[error] 5-5: Expected a qualified rule, or an at rule but instead found '}'.

Expected a qualified rule, or an at rule here.

(parse)

🔇 Additional comments (2)
emhttp/plugins/dynamix.vm.manager/sheets/AddVM.css (2)

7-8: LGTM!

Good implementation of cross-browser scrollbar styling with both standard and webkit-specific properties.


55-57: LGTM!

Good implementation of utility classes. The !important flag is acceptable for the .hidden utility class as it's a common pattern to ensure elements stay hidden.

Comment on lines +2 to +6
#vmform table{margin-top:0;table-layout:fixed}
#vmform div.title + table{margin-top:0}
#vmform table tr{vertical-align:top;line-height:40px}
#vmform table tr td:nth-child(odd){width:300px;text-align:right;padding-right:10px}
#vmform table tr td:nth-child(even){width:110px}
@media (max-width:1280px){#vmform table tr td:first-child{width:35%;padding-right:4rem}}
@media (min-width:1281px){#vmform table tr td:first-child{width:30%;padding-right:4rem}}
@media (min-width:1921px){#vmform table tr td:first-child{width:25%;padding-right:4rem}}
#vmform table tr td:last-child{width:inherit}
#vmform table tr{vertical-align:top;line-height:4rem;height:4rem}
#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}}
#vmform table tr td:nth-child(2){width:800px}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix syntax error in table cell selector.

There's an extra closing brace in the CSS rule for the first table cell.

Apply this diff to fix the syntax error:

-#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}}
+#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}

Consider responsive design for fixed widths.

The fixed width of 800px for the second column might cause horizontal scrolling on smaller screens.

Consider using relative units or media queries for better responsiveness:

-#vmform table tr td:nth-child(2){width:800px}
+#vmform table tr td:nth-child(2){width:75%}
+@media (max-width: 1024px) {
+  #vmform table tr td:nth-child(2){width:100%}
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#vmform table{margin-top:0;table-layout:fixed}
#vmform div.title + table{margin-top:0}
#vmform table tr{vertical-align:top;line-height:40px}
#vmform table tr td:nth-child(odd){width:300px;text-align:right;padding-right:10px}
#vmform table tr td:nth-child(even){width:110px}
@media (max-width:1280px){#vmform table tr td:first-child{width:35%;padding-right:4rem}}
@media (min-width:1281px){#vmform table tr td:first-child{width:30%;padding-right:4rem}}
@media (min-width:1921px){#vmform table tr td:first-child{width:25%;padding-right:4rem}}
#vmform table tr td:last-child{width:inherit}
#vmform table tr{vertical-align:top;line-height:4rem;height:4rem}
#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}}
#vmform table tr td:nth-child(2){width:800px}
#vmform table{margin-top:0;table-layout:fixed}
#vmform div.title + table{margin-top:0}
#vmform table tr{vertical-align:top;line-height:4rem;height:4rem}
#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}
#vmform table tr td:nth-child(2){width:75%}
@media (max-width: 1024px) {
#vmform table tr td:nth-child(2){width:100%}
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 5-5: Expected a qualified rule, or an at rule but instead found '}'.

Expected a qualified rule, or an at rule here.

(parse)

Comment on lines +2 to +8
#vmform table{margin-top:0;table-layout:fixed}
#vmform div.title + table{margin-top:0}
#vmform table tr{vertical-align:top;line-height:40px}
#vmform table tr td:nth-child(odd){width:300px;text-align:right;padding-right:10px}
#vmform table tr td:nth-child(even){width:110px}
@media (max-width:1280px){#vmform table tr td:first-child{width:35%;padding-right:4rem}}
@media (min-width:1281px){#vmform table tr td:first-child{width:30%;padding-right:4rem}}
@media (min-width:1921px){#vmform table tr td:first-child{width:25%;padding-right:4rem}}
#vmform table tr td:last-child{width:inherit}
#vmform table tr{vertical-align:top;line-height:4rem;height:4rem}
#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}}
#vmform table tr td:nth-child(2){width:800px}
#vmform textarea{max-width:500px;scrollbar-width:4px}
#vmform textarea::-webkit-scrollbar{height:4px;width:4px}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix syntax error and review layout changes.

The table layout changes look good, improving structure with fixed layout and consistent dimensions. However, there's a critical syntax error that needs to be fixed.

Apply this diff to fix the syntax error:

-#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}}
+#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#vmform table{margin-top:0;table-layout:fixed}
#vmform div.title + table{margin-top:0}
#vmform table tr{vertical-align:top;line-height:40px}
#vmform table tr td:nth-child(odd){width:300px;text-align:right;padding-right:10px}
#vmform table tr td:nth-child(even){width:110px}
@media (max-width:1280px){#vmform table tr td:first-child{width:35%;padding-right:4rem}}
@media (min-width:1281px){#vmform table tr td:first-child{width:30%;padding-right:4rem}}
@media (min-width:1921px){#vmform table tr td:first-child{width:25%;padding-right:4rem}}
#vmform table tr td:last-child{width:inherit}
#vmform table tr{vertical-align:top;line-height:4rem;height:4rem}
#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}}
#vmform table tr td:nth-child(2){width:800px}
#vmform textarea{max-width:500px;scrollbar-width:4px}
#vmform textarea::-webkit-scrollbar{height:4px;width:4px}
#vmform table{margin-top:0;table-layout:fixed}
#vmform div.title + table{margin-top:0}
#vmform table tr{vertical-align:top;line-height:4rem;height:4rem}
#vmform table tr td:nth-child(1){width:25%;text-align:right;padding-right:4rem}
#vmform table tr td:nth-child(2){width:800px}
#vmform textarea{max-width:500px;scrollbar-width:4px}
#vmform textarea::-webkit-scrollbar{height:4px;width:4px}
🧰 Tools
🪛 Biome (1.9.4)

[error] 5-5: Expected a qualified rule, or an at rule but instead found '}'.

Expected a qualified rule, or an at rule here.

(parse)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants